-
Notifications
You must be signed in to change notification settings - Fork 440
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Local environment variables #252 #366
Conversation
@mtnrbq thanks for your nice patch, but please consolidate the variable naming like variable name in code and README, and also fix the tslint errors. Thanks in advance. |
Will do, can you provide an example re variable naming, so I am clear (I thought I had followed existing, apologies)
Get Outlook for Android<https://aka.ms/ghei36>
…________________________________
From: Huachao Mao <notifications@github.com>
Sent: Monday, May 27, 2019 12:14:04 PM
To: Huachao/vscode-restclient
Cc: mtnrbq; Mention
Subject: Re: [Huachao/vscode-restclient] Local environment variables #252 (#366)
@mtnrbq<https://github.com/mtnrbq> thanks for your nice patch, but please consolidate the variable naming like variable name in code and README, and also fix the tslint errors. Thanks in advance.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#366?email_source=notifications&email_token=AAXERXLACVJKYXRZ5IKRXJTPXM7WZA5CNFSM4HPWFTTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWITI7Q#issuecomment-496055422>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAXERXI4AOHDOUSLVHVYZ33PXM7WZANCNFSM4HPWFTTA>.
|
@mtnrbq I don't care about the naming convention, my concern is the naming consistency in your own code and doc like following: BTW, still thanks for your great patch. |
Ahh , got you. All good, and I will address these.
I wanted to contribute back, as I've gotten a lot of value from the extension 😃
Get Outlook for Android<https://aka.ms/ghei36>
…________________________________
From: Huachao Mao <notifications@github.com>
Sent: Monday, May 27, 2019 1:01:30 PM
To: Huachao/vscode-restclient
Cc: mtnrbq; Mention
Subject: Re: [Huachao/vscode-restclient] Local environment variables #252 (#366)
@mtnrbq<https://github.com/mtnrbq> I don't care about the naming convention, my concern is the naming consistency in your own code and doc like following:
In the README, the new system variable name is $processEnvName, while in the source code it seems to be processEnv. And in the README, the environment variable sample you provided is secretKey, while in the usage part seems to be secret. I just want to make the code and doc no ambiguous.
BTW, still thanks for your great patch.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#366?email_source=notifications&email_token=AAXERXLO2JYOMMZJ5HRS47LPXNFIVA5CNFSM4HPWFTTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWIU5II#issuecomment-496062113>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAXERXJZC3E5FNCZ6EAKO73PXNFIVANCNFSM4HPWFTTA>.
|
Adjust sync-spin icon and text location
@Huachao - documentation updated, and code linted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to touch the package-lock.json?
@Huachao - reverted changes to package-lock and removed spurious space |
README.md
Outdated
@@ -447,10 +451,54 @@ System variables provide a pre-defined set of variables that can be used in any | |||
|
|||
`public|cn|de|us|ppe`: Optional. Specify top-level domain (TLD) to get a token for the specified government cloud, `public` for the public cloud, or `ppe` for internal testing. Default: TLD of the REST endpoint; `public` if not valid. | |||
|
|||
`<domain|tenantId>`: Optional. Domain or tenant id for the directory to sign in to. Default: Pick a directory from a drop-down or press `Esc` to use the home directory (`common` for Microsoft Account). | |||
`<domain|tenantId>` : Optional. Domain or tenant id for the directory to sign in to. Default: Pick a directory from a drop-down or press `Esc` to use the home directory (`common` for Microsoft Account). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space removed
README.md
Outdated
|
||
`aud:<domain|tenantId>`: Optional. Target Azure AD app id (aka client id) or domain the token should be created for (aka audience or resource). Default: Domain of the REST endpoint. | ||
* `{{$guid}}`: Add a RFC 4122 v4 UUID | ||
* `{{$processEnv [%]envVarName}}`: Allows the resolution of local machine environment variables to string values. A typical use case is for secret keys that you don't want to commit to source control. | ||
For example: define shell environment variable | ||
in `.bashrc` or similar on windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary to break into two separate lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed line break
README.md
Outdated
|
||
`aud:<domain|tenantId>`: Optional. Target Azure AD app id (aka client id) or domain the token should be created for (aka audience or resource). Default: Domain of the REST endpoint. | ||
* `{{$guid}}`: Add a RFC 4122 v4 UUID | ||
* `{{$processEnv [%]envVarName}}`: Allows the resolution of local machine environment variables to string values. A typical use case is for secret keys that you don't want to commit to source control. | ||
For example: define shell environment variable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made singular
"local": { | ||
"version": "v2", | ||
"host": "localhost", | ||
"secretKey": "DEVSECRET" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
secret?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now is secretKey in all references
}, | ||
"production": { | ||
"host": "example.com", | ||
"secretKey" : "PRODSECRET" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now is secretKey in all references
``` | ||
or, it can be rewritten to indirectly refer to the key using an extension environment setting (e.g. ```%secret```) to be environment independent using the optional ```%``` modifier. | ||
```http | ||
### Use secretKey from extension environment settings to determine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
secret
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now is secretKey in all references
@mtnrbq another minor suggestion, add the |
@mtnrbq I have added some comments and thanks for your contribution |
return name; | ||
} | ||
} else { | ||
return name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also revert the package-lock.json
@@ -343,17 +343,19 @@ For environment variables, each environment comprises a set of key value pairs d | |||
"local": { | |||
"version": "v2", | |||
"host": "localhost", | |||
"token": "test token" | |||
"token": "test token", | |||
"secret": "devSecret" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so consolidate with other references
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Huachao - sorry I'm not quite following your comment.
Would it be worthwhile to merge this PR as is and raise new issue on document clarifications?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Package-lock.json reverted
README.md
Outdated
```http | ||
### Use secretKey from extension environment settings to determine | ||
### which local machine environment variable to use | ||
<<<<<<< HEAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meged, thanks
Provide access to local environment variables #252
{{$processEnv [%]envVarName}}
: Allows the resolution of local machine environment variables to string values. A typical use case is for secret keys that you don't want to commit to source control.For example: define shell environment variable
in
.bashrc
or similar on windowsand with extension setting environment variables.
You can refer directly to the key (e.g.
PRODSECRET
) in the script, for example if running in the production environmentor, it can be rewritten to indirectly refer to the key using an extension environment setting (e.g.
%secret
) to be environment independent using the optional%
modifier.envVarName
: Mandatory. Specifies the local machine environment variable%
: Optional. If specified, treats envVarName as an extension setting environment variable, and uses the value of that for the lookup.